feat: support manifest&list writer part 1 by dongxiao1198 · Pull Request #169 · apache/iceberg-cpp

dongxiao1198 · 2025-08-12T07:30:05Z

1 add avro writer and factory(without write func since converter pull/166 not finished yet)
2 add manifest and manifest list writer internal implement definition

1 add avro writer and factory(without write func since converter pull/166 not finished yet) 2 add manifest and manifest list writer internal implement definition

wgtmac

I'd suggest split this PR into separate ones to make review easier. Perhaps one for avro writer, one for manifest list writer, and one for manifest file writer.

wgtmac · 2025-08-13T06:26:42Z

src/iceberg/metrics.h

+class ICEBERG_EXPORT Metrics {
+ public:
+  Metrics() = default;
+
+  explicit Metrics(int64_t row_count,
+                   std::unordered_map<int64_t, int64_t> column_sizes = {},
+                   std::unordered_map<int64_t, int64_t> value_counts = {},
+                   std::unordered_map<int64_t, int64_t> null_value_counts = {},
+                   std::unordered_map<int64_t, int64_t> nan_value_counts = {},
+                   std::unordered_map<int64_t, Literal> lower_bounds = {},
+                   std::unordered_map<int64_t, Literal> upper_bounds = {})
+      : row_count_(row_count),
+        column_sizes_(std::move(column_sizes)),
+        value_counts_(std::move(value_counts)),
+        null_value_counts_(std::move(null_value_counts)),
+        nan_value_counts_(std::move(nan_value_counts)),
+        lower_bounds_(std::move(lower_bounds)),
+        upper_bounds_(std::move(upper_bounds)) {}
+
+ private:
+  int64_t row_count_ = 0;
+  std::unordered_map<int64_t, int64_t> column_sizes_;
+  std::unordered_map<int64_t, int64_t> value_counts_;
+  std::unordered_map<int64_t, int64_t> null_value_counts_;
+  std::unordered_map<int64_t, int64_t> nan_value_counts_;
+  std::unordered_map<int64_t, Literal> lower_bounds_;
+  std::unordered_map<int64_t, Literal> upper_bounds_;
+};


Suggested change

class ICEBERG_EXPORT Metrics {

public:

Metrics() = default;

explicit Metrics(int64_t row_count,

std::unordered_map<int64_t, int64_t> column_sizes = {},

std::unordered_map<int64_t, int64_t> value_counts = {},

std::unordered_map<int64_t, int64_t> null_value_counts = {},

std::unordered_map<int64_t, int64_t> nan_value_counts = {},

std::unordered_map<int64_t, Literal> lower_bounds = {},

std::unordered_map<int64_t, Literal> upper_bounds = {})

: row_count_(row_count),

column_sizes_(std::move(column_sizes)),

value_counts_(std::move(value_counts)),

null_value_counts_(std::move(null_value_counts)),

nan_value_counts_(std::move(nan_value_counts)),

lower_bounds_(std::move(lower_bounds)),

upper_bounds_(std::move(upper_bounds)) {}

private:

int64_t row_count_ = 0;

std::unordered_map<int64_t, int64_t> column_sizes_;

std::unordered_map<int64_t, int64_t> value_counts_;

std::unordered_map<int64_t, int64_t> null_value_counts_;

std::unordered_map<int64_t, int64_t> nan_value_counts_;

std::unordered_map<int64_t, Literal> lower_bounds_;

std::unordered_map<int64_t, Literal> upper_bounds_;

};

struct ICEBERG_EXPORT Metrics {

int64_t row_count = 0;

std::unordered_map<int64_t, int64_t> column_sizes;

std::unordered_map<int64_t, int64_t> value_counts;

std::unordered_map<int64_t, int64_t> null_value_counts;

std::unordered_map<int64_t, int64_t> nan_value_counts;

std::unordered_map<int64_t, Literal> lower_bounds;

std::unordered_map<int64_t, Literal> upper_bounds;

};

What about making it a simple struct to enable aggregate initialization?

wgtmac · 2025-08-13T06:27:20Z

src/iceberg/metrics.h

+
+#pragma once
+
+/// \file iceberg/metrics.h


Add some comment here? e.g. Iceberg file format metrics

wgtmac · 2025-08-13T06:29:14Z

src/iceberg/manifest_writer.h

      const std::vector<ManifestEntry>& entries) const = 0;

+  /// \brief Close writer and flush to storage.
+  virtual void Close() = 0;


Suggested change

virtual void Close() = 0;

virtual Status Close() = 0;

wgtmac · 2025-08-13T06:29:22Z

src/iceberg/manifest_writer.h

  virtual Status WriteManifestFiles(const std::vector<ManifestFile>& files) const = 0;

+  /// \brief Close writer and flush to storage.
+  virtual void Close() = 0;


Suggested change

virtual void Close() = 0;

virtual Status Close() = 0;

wgtmac · 2025-08-13T06:33:53Z

src/iceberg/file_writer.h

+  /// When available, this information is used for planning scan tasks whose boundaries
+  /// are determined by these offsets. The returned list must be sorted in ascending order
+  /// Only valid after the file is closed.
+  virtual std::vector<int64_t> splitOffsets() = 0;


Suggested change

virtual std::vector<int64_t> splitOffsets() = 0;

virtual std::vector<int64_t> split_offsets() = 0;

wgtmac · 2025-08-13T06:34:50Z

src/iceberg/file_writer.h

+  /// \brief Get the file statistics.
+  virtual std::shared_ptr<Metrics> metrics() = 0;
+
+  /// \brief Get the file length.


Suggested change

/// \brief Get the file length.

/// \brief Get the file length.

/// Only valid after the file is closed.

wgtmac · 2025-08-13T06:35:16Z

src/iceberg/file_writer.h

  /// \return Status of write results.
  virtual Status Write(ArrowArray data) = 0;
+
+  /// \brief Get the file statistics.


Suggested change

/// \brief Get the file statistics.

/// \brief Get the file statistics.

/// Only valid after the file is closed.

wgtmac · 2025-08-13T06:36:01Z

src/iceberg/file_writer.h

  virtual Status Write(ArrowArray data) = 0;
+
+  /// \brief Get the file statistics.
+  virtual std::shared_ptr<Metrics> metrics() = 0;


Suggested change

virtual std::shared_ptr<Metrics> metrics() = 0;

virtual Metrics metrics() = 0;

Perhaps we can just return a simple struct instead of a shared_ptr?

wgtmac · 2025-08-13T06:39:24Z

src/iceberg/avro/avro_writer.cc

+}  // namespace
+
+// A stateful context to keep track of the writing progress.
+struct WriteContext {};


Do we really need a context? Writer is much simpler than the reader impl.

wgtmac · 2025-08-13T06:44:59Z

src/iceberg/avro/avro_writer.cc

+    auto root = std::make_shared<::avro::NodeRecord>();
+    ToAvroNodeVisitor visitor;
+    for (const auto& field : write_schema_->fields()) {
+      ::avro::NodePtr node;
+      ICEBERG_RETURN_UNEXPECTED(visitor.Visit(field, &node));
+      root->addLeaf(node);
+    }


Suggested change

auto root = std::make_shared<::avro::NodeRecord>();

ToAvroNodeVisitor visitor;

for (const auto& field : write_schema_->fields()) {

::avro::NodePtr node;

ICEBERG_RETURN_UNEXPECTED(visitor.Visit(field, &node));

root->addLeaf(node);

}

::avro::NodePtr root;

ICEBERG_RETURN_UNEXPECTED(ToAvroNodeVisitor{}.Visit(*write_schema_, &root));

dongxiao1198 · 2025-08-13T06:55:19Z

add avro writer and factory(without write func since converter pull/166 not finished yet)

ok, I will seperate them into 2 pr

xiao.dong added 4 commits August 12, 2025 15:29

feat: support manifest&list writer part 1

bcd4066

1 add avro writer and factory(without write func since converter pull/166 not finished yet) 2 add manifest and manifest list writer internal implement definition

fix schema

d50a20c

add file metric in writer

c7e52a0

fix warning

88ecbb8

wgtmac reviewed Aug 13, 2025

View reviewed changes

dongxiao1198 closed this Aug 13, 2025

dongxiao1198 deleted the manifest_writer_impl branch December 5, 2025 03:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support manifest&list writer part 1#169

feat: support manifest&list writer part 1#169
dongxiao1198 wants to merge 4 commits intoapache:mainfrom
dongxiao1198:manifest_writer_impl

dongxiao1198 commented Aug 12, 2025

Uh oh!

wgtmac left a comment

Uh oh!

wgtmac Aug 13, 2025

Uh oh!

wgtmac Aug 13, 2025

Uh oh!

wgtmac Aug 13, 2025

Uh oh!

wgtmac Aug 13, 2025

Uh oh!

wgtmac Aug 13, 2025

Uh oh!

wgtmac Aug 13, 2025

Uh oh!

wgtmac Aug 13, 2025

Uh oh!

wgtmac Aug 13, 2025

Uh oh!

wgtmac Aug 13, 2025

Uh oh!

wgtmac Aug 13, 2025

Uh oh!

dongxiao1198 commented Aug 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	virtual std::vector<int64_t> splitOffsets() = 0;
	virtual std::vector<int64_t> split_offsets() = 0;

	/// \brief Get the file length.
	/// \brief Get the file length.
	/// Only valid after the file is closed.

	/// \brief Get the file statistics.
	/// \brief Get the file statistics.
	/// Only valid after the file is closed.

	virtual std::shared_ptr<Metrics> metrics() = 0;
	virtual Metrics metrics() = 0;

Conversation

dongxiao1198 commented Aug 12, 2025

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongxiao1198 commented Aug 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants